Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Mailpit and Xlight in class.action.switchVersion.php and update icon paths in class.winbinder.php and class.core.php #82

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Add support for Mailpit and Xlight in class.action.switchVersion.php and update icon paths in class.winbinder.php and class.core.php #82

merged 1 commit into from
Sep 7, 2024

Conversation

N6REJ
Copy link
Contributor

@N6REJ N6REJ commented Sep 6, 2024 •
edited by qodo-merge-pro bot
Loading

PR Type

enhancement, bug fix

Description

  • Added support for Mailpit and Xlight services in class.action.switchVersion.php, enhancing the version switching capabilities.
  • Updated icon paths in class.core.php and class.winbinder.php to include the 'homepage' directory, ensuring correct resource loading.
  • Improved code formatting and added comments for better readability and maintainability.

Changes walkthrough 📝

Relevant files
Enhancement
class.action.switchVersion.php
Add support for Mailpit and Xlight in version switcher     

core/classes/actions/class.action.switchVersion.php

  • Added support for Mailpit and Xlight services.
  • Improved code formatting for better readability.
  • Enhanced service switching logic.
    		•  +90/-61 
    class.core.php
    Update icon paths and add image path retrieval                     

    core/classes/class.core.php

  • Updated icon paths to include 'homepage' directory.
  • Added a new method to retrieve image paths.
    		•  +15/-1   
    Bug fix
    class.winbinder.php
    Update icon paths and improve code comments                           

    core/classes/class.winbinder.php

  • Updated icon paths to use getIconsPath method.
  • Improved code formatting and comments.
    		•  +9/-7     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @N6REJ
    Add support for Mailpit and Xlight in `class.action.switchVersion.php…
    c60952a 
    …` and update icon paths in `class.winbinder.php` and `class.core.php`
    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Sep 6, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    There's repetitive code for setting up pathsToScan for different services. This could be refactored into a separate method to reduce duplication.

    Missing Error Handling
    The constructor doesn't handle the case where an invalid service name is provided. It should include an else clause to handle unknown services.

    TODO Comment
    There's a TODO comment about undeclared constants. This should be addressed or removed if no longer relevant.

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Sep 6, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract repetitive code for setting up paths to scan into a separate method to reduce duplication

    Extract the repetitive code for setting up $this->pathsToScan into a separate method
    to reduce code duplication.

    core/classes/actions/class.action.switchVersion.php [48-54]

    -foreach ($folderList as $folder) {
-    $this->pathsToScan[] = array(
-        'path'      => $bearsamppBins->getApache()->getRootPath() . '/' . $folder,
-        'includes'  => array('.ini', '.conf'),
-        'recursive' => true
-    );
+$this->setupPathsToScan($bearsamppBins->getApache()->getRootPath(), $folderList, ['.ini', '.conf'], true);
+
+// Add this method to the class:
+private function setupPathsToScan($rootPath, $folderList, $includes, $recursive) {
+    foreach ($folderList as $folder) {
+        $this->pathsToScan[] = array(
+            'path'      => $rootPath . '/' . $folder,
+            'includes'  => $includes,
+            'recursive' => $recursive
+        );
+    }
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Extracting repetitive code into a separate method reduces duplication, enhances readability, and makes the code easier to maintain and update.

    		9
    Enhancement
    Replace multiple if-else conditions with a switch statement for better code structure and readability

    Consider using a switch statement instead of multiple if-else conditions for better
    readability and maintainability.

    core/classes/actions/class.action.switchVersion.php [42-169]

    -if ($args[0] == $bearsamppBins->getApache()->getName()) {
-    $this->bin            = $bearsamppBins->getApache();
-    $this->currentVersion = $bearsamppBins->getApache()->getVersion();
-    $this->service        = $bearsamppBins->getApache()->getService();
-    $this->changePort     = true;
-    $folderList           = Util::getFolderList($bearsamppBins->getApache()->getRootPath());
-    foreach ($folderList as $folder) {
-        $this->pathsToScan[] = array(
-            'path'      => $bearsamppBins->getApache()->getRootPath() . '/' . $folder,
-            'includes'  => array('.ini', '.conf'),
-            'recursive' => true
-        );
-    }
-} elseif ($args[0] == $bearsamppBins->getPhp()->getName()) {
-    // ... (similar blocks for other conditions)
+switch ($args[0]) {
+    case $bearsamppBins->getApache()->getName():
+        $this->bin            = $bearsamppBins->getApache();
+        $this->currentVersion = $bearsamppBins->getApache()->getVersion();
+        $this->service        = $bearsamppBins->getApache()->getService();
+        $this->changePort     = true;
+        $folderList           = Util::getFolderList($bearsamppBins->getApache()->getRootPath());
+        foreach ($folderList as $folder) {
+            $this->pathsToScan[] = array(
+                'path'      => $bearsamppBins->getApache()->getRootPath() . '/' . $folder,
+                'includes'  => array('.ini', '.conf'),
+                'recursive' => true
+            );
+        }
+        break;
+    case $bearsamppBins->getPhp()->getName():
+        // ... (similar blocks for other conditions)
+        break;
+    // ... (other cases)
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a switch statement instead of multiple if-else conditions can significantly improve code readability and maintainability, especially when dealing with numerous conditions as in this case.

    		8
    Possible issue
    Add error handling for setting the message box icon to handle potential missing icon files

    Consider adding error handling for the setImage method call to gracefully handle
    cases where the icon file might be missing.

    core/classes/class.winbinder.php [918]

    -$this->setImage( $messageBox, $bearsamppCore->getIconsPath() . '/app.ico' );
+$iconPath = $bearsamppCore->getIconsPath() . '/app.ico';
+if (file_exists($iconPath)) {
+    $this->setImage($messageBox, $iconPath);
+} else {
+    // Log the missing icon file or use a default icon
+    error_log("App icon not found: $iconPath");
+}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the setImage method enhances robustness by ensuring the application can handle missing icon files gracefully, which is a potential issue that could disrupt user experience.

    		8
    Best practice
    Use a constant for the app icon filename to improve maintainability

    Use a constant for the 'app.ico' filename to avoid repeating the string and make it
    easier to update if needed.

    core/classes/class.winbinder.php [147]

    -$this->setImage( $window, $bearsamppCore->getIconsPath() . '/app.ico' );
+const APP_ICON = 'app.ico';
+// ...
+$this->setImage( $window, $bearsamppCore->getIconsPath() . '/' . self::APP_ICON );
    Suggestion importance[1-10]: 7

    Why: Using a constant for the app icon filename improves maintainability by centralizing the string, making it easier to update and reducing the risk of typos.

    		7

    @jwaisner jwaisner merged commit b13a5f3 into main Sep 7, 2024
    1 check passed
    @jwaisner jwaisner deleted the switcher branch September 7, 2024 04:55
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Bug fix bug_fix enhancement New feature or request Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @N6REJ @jwaisner